Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter facets #18

Merged
merged 9 commits into from
Sep 9, 2019
Merged

Filter facets #18

merged 9 commits into from
Sep 9, 2019

Conversation

cavis
Copy link
Member

@cavis cavis commented Aug 26, 2019

Adds filter facet attributes into the paged collection. It's up to the client to know how to interpret them, and change the ?filter param.

Also adds a vary link to paged collections, which gives you a templated link to this collection.

{
  "count": 25,
  "total": 71,
  "facets": {
    "type": [
      {"id": "the_normal_type", "label": "The Normal Type", "count": 10},
      {"id": "a_different_type", "label": "A Different Type", "count": 7}
    ],
    "status": [
      {"id": "active", "label": "Active", "count": 9},
      {"id": "inactive", "label": "Inactive", "count": 15}
    ],
    "some_custom_filter": [
      {"id": "", "label": "", "count": 12}
    ]
  },
  "_embedded": {
    "prx:items": []
  },
  "_links": {
    "self": {
      "href": "/api/v1/campaigns?filters=text%3Dwhatever",
      "profile": "http://meta.prx.org/model/collection/application-record/campaign"
    },
    "vary": {
      "href": "/api/v1/campaigns{?page,per,zoom,filters,sorts}",
      "templated": true
    }
  }
}

The idea with facets is that they change as you apply filters, and represent what counts would return if you applied that filter+value in addition to any filters you currently have applied. There is nothing in the JSON to tell you what filters are currently applied (unless you parse it out of the self link). So the client had better be tracking its own filter state.

The vary link only appears if there are query params to vary (set by the vary_params method). By default here, base HalApi::Representers do not have a vary link (though you could argue we should allow varying by ?zoom). And paged representers do have a vary link, including ?page,per,zoom,filters,sorts which are all part of the HalApi::Controller by default.

@cavis cavis self-assigned this Aug 26, 2019
@cavis cavis changed the title Filter/sort facets Filter facets Aug 27, 2019
@@ -53,6 +53,28 @@ def filters
@filters ||= parse_filters_param
end

def filter_facets
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, the facets attribute won't render at all. You have to provide this method in your controller.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me - how does the API indicate which fields can be used as facets?
e.g. type, status, etc.?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specifics of this were implemented in Augury (i.e. left up to the specific app). Ended up being a hash of "facet name" (which is the ?filters= key name) to a list of options for the facet.

@@ -75,6 +75,10 @@ def resources_base
self.class.resource_class.where(nil)
end

def resources_query
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convenience method for controllers implementing filter_facets. For instance, in the Augury campaigns controller I found myself doing:

def filter_facets
  {
    type: resources_query.group(:type).count,
    status: resources_query.group(:status).count,
  }
end

@@ -36,6 +37,14 @@ def self_url(represented)
href_url_helper(represented.params)
end

def vary_url(represented)
href_url_helper(represented.params.except(*vary_params))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was necessary to remove any of the "varied query params" before getting the url.

@@ -46,6 +55,7 @@ def profile_url(represented)
# if it is a lambda, execute in the context against the represented.parent (if there is one) or represented
def href_url_helper(options={})
if represented_url.nil?
options = options.except(:format)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an existing bug/annoyance where paged collections always had a self link with .json in it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yuck, good fix!

@cavis cavis requested a review from kookster August 27, 2019 18:57
Copy link
Member

@kookster kookster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have questions.

@@ -53,6 +53,28 @@ def filters
@filters ||= parse_filters_param
end

def filter_facets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me - how does the API indicate which fields can be used as facets?
e.g. type, status, etc.?

end

def index_collection
collection = defined?(super) ? super : HalApi::PagedCollection.new([])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda thought some of this might end up in or extending paged collection, but I can see the separation of the controller knowing about the facets and setting them, like with other model data. I wonder if the controller or the paged collection should be more aware of the structure of it though...probably not, probably overkill to have mutator and other methods on the collection...maybe a method to add/set a facet?

I guess I am unsure where to put all the filtering functionality. I think your move here to keep it in a concern/mixin may actually be better, as it keeps most of the logic in the same place.

Copy link
Member Author

@cavis cavis Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see doing an optimization like that. But to start, I wanted to leave that up to the implementing app (augury.prx.org in this case) and see where that takes us. Rather than locking them into a more controlled data structure.

collection = defined?(super) ? super : HalApi::PagedCollection.new([])

# add facets if defined, removing filters/facets with counts of 0
non_zero_facets = (filter_facets || {}).with_indifferent_access.tap do |hash|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the filter_key have more than just an id? Should there be a human readable label for it?
e.g. some_custom_filter would not be the best for display.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have talked myself out of this, withdrawn!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd lump this in with "describing all available filters", which is sort of out-of-scope for this PR now. Opened #19.

@@ -46,6 +55,7 @@ def profile_url(represented)
# if it is a lambda, execute in the context against the represented.parent (if there is one) or represented
def href_url_helper(options={})
if represented_url.nil?
options = options.except(:format)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yuck, good fix!

Copy link
Member

@kookster kookster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

questions answered - lgtm

@cavis cavis merged commit 8f3eaee into master Sep 9, 2019
@cavis cavis deleted the feat/filter_sort_facets branch September 9, 2019 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants